http: resume kept-alive when no body allowed by mweberxyz · Pull Request #52329 · nodejs/node

mweberxyz · 2024-04-02T17:41:06Z

In accordance with https://www.rfc-editor.org/rfc/rfc9112#name-message-body-length: HEAD, 1xx, 204, and 304 responses cannot contain a message body.

If a connection will be kept-alive, resume the socket during parsing so that it may be returned to the free pool, even if the caller has not consumed the known-expected-empty response.

Fixes: #47228

Note: The original source of the issue reporter's issue was due to failure to consume the response body of a 3xx request, which this change will not resolve. As an option, 3xx with a Content-Length of 0 could receive the same auto-resume treatment. I am hesitant to propose that change, as some HTTP servers may return a message body (ie <a href=here>Moved Here</a>), which would then make the caller's responsibility to consume or resume dependent on the particular remote server's behavior.

nodejs-github-bot · 2024-04-02T17:41:11Z

Review requested:

@nodejs/http
@nodejs/net

ShogunPanda

Nit. Rest LGTM.

ShogunPanda · 2024-04-05T06:47:38Z

+    if (!shouldKeepAlive) {
+      req.shouldKeepAlive = false;
+    }
+


Please remove this space.

mweberxyz · 2024-04-05T15:54:50Z

Setting to draft, went to fix review nit and add tests and I am not convinced issue is fully resolved.

mweberxyz · 2024-04-05T17:54:10Z

@ShogunPanda I added tests and discovered that my approach putting the logic in in an else if following the !shouldKeepAlive condition will work for 204 and 304s, but not for HEAD, because at this point shouldKeepAlive is set to false for HEAD requests -- this is coming from llhttp.

mcollina

lgtm

ronag · 2024-04-09T10:21:08Z

This is potentially unsafe. Some servers don't properly end HEAD requests and there is no way to detect it. Such a case would potentially corrupt following responses from the same connection. Which is why in undici we always reset the connection after a HEAD request, unless explicitly told otherwise (reset: false option).

mweberxyz · 2024-04-09T16:11:00Z

@ronag re-using a socket following a HEAD request is the existing logic.

I checked with the following replication:

const http = require("http");

const noKeepalive = process.argv.at(-1) === "no-keepalive";

const opts = {};

if (noKeepalive) {
  opts.agent = new http.Agent({ keepAlive: false });
}

console.log({
  version: process.version,
  keepAlive: !noKeepalive,
});

http.get("http://www.example.com", { method: "HEAD", ...opts }, (res) => {
  res.resume();
  const headSocket = res.socket;
  setTimeout(() => {
    http.get("http://www.example.com", { method: "GET", ...opts }, (res2) => {
      console.log({ sameSocket: headSocket === res2.socket });
    });
  }, 500);
});

gives:

% node index.js keepalive
{ version: 'v20.12.1', keepAlive: true }
{ sameSocket: true }

and

% node index.js no-keepalive
{ version: 'v20.12.1', keepAlive: false }
{ sameSocket: false }

Should I take a look at that first before returning to this PR?

ronag · 2024-04-09T16:16:13Z

What does this PR do then?

ronag · 2024-04-09T16:17:02Z

I think the problem might apply to all expected empty responses. I need to check that for undici as well.

mweberxyz · 2024-04-09T16:19:25Z

What does this PR do then?

Makes it so the res.resume() following a HEAD request, a request that returns a 204, or a request that returns a 304 is implicit.

ronag · 2024-04-09T16:22:36Z

Makes it so the res.resume() following a HEAD request, a request that returns a 204, or a request that returns a 304 is implicit.

Sorry, I don't understand.

ronag

Now I get it. LGTM.

mweberxyz · 2024-04-09T16:26:36Z

FWIW what lead me here is a package that was using node:https and not consuming the response on a request that returned a 302, which prevented node from exiting until the keepAlive timed out. I can't fix that because a 302 could return a response, so it should be up to the caller to avoid keeping node from exiting, but figured in these three cases where an http request could not possibly return a response, per spec, we can safely do this automatically.

Will push changes for lint errors shortly.

mweberxyz · 2024-04-09T18:49:27Z

In a twist of irony, my test appears to have left a socket open. Will take a look and push fix.

mweberxyz · 2024-04-09T20:04:25Z

Ready for ci: the server in the test needed to be unref'd.

ShogunPanda

LGTM!

mweberxyz · 2024-04-10T15:11:41Z

I can't replicate that final test-asan failure locally - let me know if there's anything else I need to do to get this ready to land 👍

nodejs-github-bot · 2025-07-11T07:24:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/67928/

nodejs-github-bot · 2025-07-28T22:04:56Z

Commit Queue failed

- Loading data for nodejs/node/pull/52329
✔  Done loading data for nodejs/node/pull/52329
----------------------------------- PR info ------------------------------------
Title      http: resume kept-alive when no body allowed (#52329)
Author     Matt Weber <hello@mweber.xyz> (@mweberxyz, first-time contributor)
Branch     mweberxyz:http-keep-alive-auto-resume -> nodejs:main
Labels     http, semver-minor, author ready, needs-ci
Commits    1
 - http: resume kept-alive when no body allowed
Committers 1
 - Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/52329
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52329
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 02 Apr 2024 17:41:06 GMT
   ✔  Approvals: 5
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/52329#pullrequestreview-1991621837
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52329#pullrequestreview-2722000587
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/52329#pullrequestreview-1989549614
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/52329#pullrequestreview-2228985328
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52329#pullrequestreview-2744925503
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-07-11T07:24:00Z: https://ci.nodejs.org/job/node-test-pull-request/67928/
- Querying data for job/node-test-pull-request/67928/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @mweberxyz(hello@mweber.xyz)
   ⚠  - commit a38c7aeac7f6 is authored by 1062734+mweberxyz@users.noreply.github.com
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/16581371844

mcollina

lgtm

nodejs-github-bot · 2025-07-29T13:14:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68296/

nodejs-github-bot · 2025-08-27T10:48:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68917/

nodejs-github-bot · 2025-09-24T13:58:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69353/

nodejs-github-bot · 2026-02-01T13:25:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/71153/

Ethan-Arrowood · 2026-07-15T14:19:58Z

Sorry to see this stall out. Can you rebase and see what might be failing in CI? Would love to help you land this!

According to RFC9112 section 6.3.1: HEAD requests, and responses with status 204 and 304 cannot contain a message body, If a socket will be kept-alive, resume the socket during parsing so that it may be returned to the free pool. Fixes nodejs#47228

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Apr 2, 2024

ShogunPanda approved these changes Apr 5, 2024

View reviewed changes

mweberxyz marked this pull request as draft April 5, 2024 15:53

mweberxyz force-pushed the http-keep-alive-auto-resume branch from 2de97af to be6b085 Compare April 5, 2024 17:52

mweberxyz marked this pull request as ready for review April 5, 2024 17:52

mweberxyz requested a review from ShogunPanda April 5, 2024 17:55

mweberxyz force-pushed the http-keep-alive-auto-resume branch from be6b085 to 2dee82a Compare April 5, 2024 19:06

mcollina approved these changes Apr 9, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2024

ronag approved these changes Apr 9, 2024

View reviewed changes

mweberxyz force-pushed the http-keep-alive-auto-resume branch from 2dee82a to 8c4db34 Compare April 9, 2024 16:41

mweberxyz force-pushed the http-keep-alive-auto-resume branch from 8c4db34 to cf6677a Compare April 9, 2024 20:03

ShogunPanda approved these changes Apr 10, 2024

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2024

This comment was marked as outdated.

Sign in to view

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 28, 2025

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 28, 2025

mcollina approved these changes Jul 29, 2025

View reviewed changes

mcollina added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jul 29, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2025

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2025

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2025

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 1, 2026

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 1, 2026

coderabbitai Bot mentioned this pull request Jun 22, 2026

Support for custom scheme in connection URI. che-incubator/che-code#723

Merged

coderabbitai Bot mentioned this pull request Jul 11, 2026

feat(release): require activation proof before v1.0.4 publish electricsheephq/evaos-code-review-bot-neondiff#536

Merged

coderabbitai Bot mentioned this pull request Jul 19, 2026

fix(build): resolve ERR_MODULE_NOT_FOUND from tsc-alias missing .js extensions thecats1105/discord-rpc-selfbot#9

Merged

4 tasks

This was referenced Jul 26, 2026

feat(stackstack): add the Safe Exposure Challenge susumutomita/TenkaCloudChallenge#306

Merged

feat: report a QuickAdd failure once, to the right person, with the message that helps chhoumann/quickadd#1617

Merged

aduh95 force-pushed the http-keep-alive-auto-resume branch from a38c7ae to b5c8c84 Compare August 5, 2026 15:43

aduh95 reviewed Aug 5, 2026

View reviewed changes

Comment thread test/parallel/test-http-agent-exit.js Outdated

aduh95 reviewed Aug 5, 2026

View reviewed changes

Comment thread test/parallel/test-http-agent-exit.js Outdated

squash! fix lint

16f9ab1

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Uh oh!

Uh oh!

Conversation

mweberxyz commented Apr 2, 2024 • edited by aduh95 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 2, 2024

Uh oh!

ShogunPanda left a comment

Choose a reason for hiding this comment

Uh oh!

ShogunPanda Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

mweberxyz commented Apr 5, 2024

Uh oh!

mweberxyz commented Apr 5, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mweberxyz commented Apr 9, 2024

Uh oh!

ronag commented Apr 9, 2024

Uh oh!

ronag commented Apr 9, 2024

Uh oh!

mweberxyz commented Apr 9, 2024

Uh oh!

ronag commented Apr 9, 2024

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

mweberxyz commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mweberxyz commented Apr 9, 2024

Uh oh!

mweberxyz commented Apr 9, 2024

Uh oh!

ShogunPanda left a comment

Choose a reason for hiding this comment

Uh oh!

mweberxyz commented Apr 10, 2024

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Jul 11, 2025

Uh oh!

nodejs-github-bot commented Jul 28, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 29, 2025

Uh oh!

nodejs-github-bot commented Aug 27, 2025

Uh oh!

nodejs-github-bot commented Sep 24, 2025

Uh oh!

nodejs-github-bot commented Feb 1, 2026

Uh oh!

Ethan-Arrowood commented Jul 15, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

mweberxyz commented Apr 2, 2024 •

edited by aduh95

Loading

ronag commented Apr 9, 2024 •

edited

Loading

mweberxyz commented Apr 9, 2024 •

edited

Loading